Skip to content

config: un-deprecating Structs until the replacement (docs etc.) are fully ready#6346

Merged
mattklein123 merged 1 commit intoenvoyproxy:masterfrom
alyssawilk:deprecation
Mar 22, 2019
Merged

config: un-deprecating Structs until the replacement (docs etc.) are fully ready#6346
mattklein123 merged 1 commit intoenvoyproxy:masterfrom
alyssawilk:deprecation

Conversation

@alyssawilk
Copy link
Contributor

As discussed offline with folks - if we're using deprecation warnings as a signal and marking things fatal-by-default we want the replacements easy to use.

Risk Level: low
Testing: none
Docs Changes: DEPRECATED.md updated
Release Notes: n/a (only included the addition, not the deprecation)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we still want to discourage use of Struct, while delaying its actual removal?

@mattklein123
Copy link
Member

Don't we still want to discourage use of Struct, while delaying its actual removal?

Probably, but I we need to think about how to do this: [discouraged = true]? :) IMO it's fine to do this PR until we figure out a better way here?

@htuch
Copy link
Member

htuch commented Mar 21, 2019

Sure. I guess the question is what are the real issues blocking widespread adoption of Any? Are we concerned that the hashing equivalent problems are going to make this unpalatable until resolved in protobuf libs? @lizan

@alyssawilk
Copy link
Contributor Author

If we're encouraging folks via warning messages and soon-to-be-crashes to migrate over but there's no examples and the canonical config isn't updated, I think we're doing it wrong.
note, haven't checked examples and docs, I'm trusting the comment thread on this.

@moderation
Copy link
Contributor

I updated a lot of the docs for Any in #6025. I still have a TODO on the template YAML but I think the PR is fairly comprehensive.

@mattklein123
Copy link
Member

@moderation so do you think we should move forward with the deprecation? Or give it 1 more cycle?

@moderation
Copy link
Contributor

I'm a bit reluctant to weigh in as me wanting to remove deprecation notices due to my configs might be a habit only I have. Having a quick look there are only a few lingering uses of the envoy.http_connection_manager Struct left in the code base. The template YAML I mentioned and:

  • test/config/utility.cc
  • test/server/listener_manager_impl_test.cc
  • test/integration/ads_integration_test.cc
  • test/config/integration/server_xds.lds.yaml
  • test/config/integration/server.yaml
  • test/config/integration/server_unix_listener.yaml
  • test/config/integration/google_com_proxy_port_0.v2.yaml

I'm sure expanding this out for the other Any changes will make this change larger. The impact of these deprecated configs is largely developer or builder facing and shouldn't impact end users.

I did notice that https://www.envoyproxy.io/learn/ needs updating - is that source in Github somewhere?

Lastly, we should make sure that projects that integrate with Envoy, particularly Istio, are well aware of the impending deprecations.

@alyssawilk
Copy link
Contributor Author

FWIW if we think the docs are fairly comprehensive and we can fix the rest by release, the other question is for a 1 year release cycle do we want to do 2 releases with warning and 2 releases with failure? If so I'd be inclined to drop this PR and just manually tweak the output of the deprecation script (I'm happy to take that on post-release) to handle the unusual cadence.

@lizan
Copy link
Member

lizan commented Mar 21, 2019

I guess the question is what are the real issues blocking widespread adoption of Any?

One issue is obviously #6252, seems Any made it difficult to generate consistent hashing for consolidating same ApiConfigSource, see #5744 as well.

FWIW if we think the docs are fairly comprehensive and we can fix the rest by release, the other question is for a 1 year release cycle do we want to do 2 releases with warning and 2 releases with failure? If so I'd be inclined to drop this PR and just manually tweak the output of the deprecation script (I'm happy to take that on post-release) to handle the unusual cadence.

+1, I'm trying to add more docs by the time of 1.10 release.

@mattklein123
Copy link
Member

+1, I'm trying to add more docs by the time of 1.10 release.

I'm OK leaving this deprecated, though, I think fail by default is pretty harsh and we shouldn't do this? Also, if there are known hashing issues should we not deprecate until that is fixed?

@lizan
Copy link
Member

lizan commented Mar 22, 2019

I'm OK leaving this deprecated, though, I think fail by default is pretty harsh and we shouldn't do this? Also, if there are known hashing issues should we not deprecate until that is fixed?

I meant not fail by default, but leaving this deprecated until we fix the hashing issue.

@htuch
Copy link
Member

htuch commented Mar 22, 2019

@lizan the question I have is whether #6252 puts Any support in "not yet ready for widespread production use" territory. I know Istio uses it, but this has required some custom workarounds and were discovered by serious deep diving.

I'm good with removing deprecated if we feel #6252 makes us "not yet ready". If the concern is over blown and most folks can use Any today with no worries around hash stability, then we should mark it deprecated but add a whitelist exception to the fail-by-default IMHO.

@kyessenov
Copy link
Contributor

@htuch we shipped 1.1 without Any. We could not identify the cause for CPU spikes when switching from Struct to Any even after making hashing deterministic in a short amount of time we had. This needs further investigation, but it's not a trivial correctness issue.

@htuch
Copy link
Member

htuch commented Mar 22, 2019

In that case, I'm good with the original PR and removing deprecate from the Struct fields. We can't deprecate without a production ready alternative in place.

@duderino
Copy link

Here are the details @kyessenov is referring to: istio/istio#12162 You'll see several attempts to fix it, with 'fixes' that worked in some contexts and not others, until finally we punted and turned off Any by default.

@mattklein123
Copy link
Member

+1 let's merge this until Any is a production ready solution. I actually wonder if we should revert some of our config examples (cc @moderation) but I think we can leave it for now while we investigate.

@lizan
Copy link
Member

lizan commented Mar 22, 2019

Sounds good to me. We can leave examples for now.

@alyssawilk
Copy link
Contributor Author

Also before we deprecate, can we move the integration tests over? Good to have the new path be the default path covered.

htuch pushed a commit that referenced this pull request Mar 27, 2019
…cation for Any and hosts deprecation for load_assignment (#6368)

Update examples for Struct deprecation for Any

Risk Level: Low - generated configs only, no changes to code
Testing: bazel build //configs:example_configs, bazel test //test/...
Docs Changes: None required
Release Notes: None required

Fixes #6025
Replaces #6356
Related #6346

Signed-off-by: Michael Payne <michael@sooper.org>
@alyssawilk alyssawilk deleted the deprecation branch May 7, 2019 19:11
lizan pushed a commit that referenced this pull request Oct 26, 2019
Deprecate opaque Struct config, again.
Fix unit tests to use the google.protobuf.Any

Reverts PR #6346
Fixes #8403

Risk Level: Low
Testing: Unit Tests
Docs Changes: N/A
Release Notes: deprecation notice

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants